fix(plugins): report updates that completed, not ones that were queued - #460
fix(plugins): report updates that completed, not ones that were queued#460ChuckBuilds wants to merge 1 commit into
Conversation
run_scheduled_updates_with_changes() snapshotted plugin_last_update, called run_scheduled_updates(), and diffed the two to answer "whose data just changed". But run_scheduled_updates() only enqueues. The work runs on the update worker and stamps plugin_last_update there, after this method has already returned, so the two snapshots were always identical and the result was always an empty list. The only path that ever worked was the synchronous kill-switch, where update() runs inline. Vegas is the caller. That empty list is what feeds mark_plugin_updated(), which drops the cached content for a plugin whose data moved -- so a segment kept scrolling whatever it was first built from. It is the failure the coordinator's own comments describe: last night's live game still drawn as live the next morning. On a live rig: zero update ticks in twenty minutes, with weather, stocks and news all updating on schedule. The worker now records each completed update in a ledger and the call drains it, reporting what has finished since the previous poll rather than what this call enqueued. That costs one tick of latency -- Vegas polls every ~4s -- and is correct whichever side of the queue the work lands on. Failure paths are excluded: they stamp the timestamp too, to space out retries, but no fresh data exists. Verified on the rig it was found on: 0 update ticks before, 208 in twenty-five minutes after, naming real plugins. The behavioural tests here would pass with both production call sites deleted, which mutation testing caught -- they drive the ledger directly. So there is also a structural test asserting the invariant at the source: wherever a successful update stamps plugin_last_update, it must record the completion. Writing it immediately caught that _record_update_failure stamps the same field and must not be included. Mutation-checked: removing either call site, removing both, and dropping the drain's clear are all caught. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPlugin update reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change reports completed plugin updates so refreshed data can be recognized instead of remaining stale; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The bug
run_scheduled_updates_with_changes()answers "which plugins have fresh data". It did so by snapshottingplugin_last_update, callingrun_scheduled_updates(), and diffing the two.But
run_scheduled_updates()only enqueues:_enqueue_update()sets state and pushes to a queue. The update runs on the worker thread, andplugin_last_updateis stamped there — after this method has already returned. So the second snapshot was always identical to the first, and the method returned[]every time. Only the synchronous kill-switch path ever worked, which is why the wiring reads as correct.Why it matters
Vegas is the caller.
_tick_plugin_updates_for_vegas()feeds that list tomark_plugin_updated(), which drops the cached content for a plugin whose data moved. With the list permanently empty, a segment kept scrolling whatever it was first built from — exactly whatcoordinator.pywarns about in its own comments:Measured on a live 512×64 rig: zero update ticks in twenty minutes, with weather, stocks and news all updating on schedule.
The fix
Report what has completed since the last poll rather than what this call enqueued. The worker records each finished update in a small ledger; the call drains it.
That costs one tick of latency — Vegas polls roughly every 4 seconds — and is correct regardless of which side of the queue the work lands on, so it keeps working if the sync/async default ever changes.
Failure paths are deliberately excluded. They stamp
plugin_last_updatetoo, to space out retries, but no fresh data exists.Verification
On the rig where it was found: 0 update ticks before, 208 in twenty-five minutes after, naming real plugins (
ledmatrix-flights,stock-news,geochron,news…).A note on the tests, because the first version of them was worthless. The behavioural tests drive the ledger directly, so they passed with both production call sites deleted — mutation testing caught that, not review. There is now also a structural test asserting the invariant at the source: wherever a successful update stamps
plugin_last_update, it must record the completion. Writing that immediately caught something I would have missed —_record_update_failurestamps the same field and must not be included.Mutation-checked: removing either call site, removing both, and dropping the drain's
clear()are all caught.Full suite: 2066 passed, with the same 4 failures present on
main(unrelated — tmpdir, web API, state reconciliation).Summary by CodeRabbit